Skip to content

Support dot notation for :only keys in partial reloads #163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Nov 21, 2024

Conversation

bknoles
Copy link
Collaborator

@bknoles bknoles commented Nov 14, 2024

Resolves #157

I also refactored the rendering logic a bit, to:

  1. Only traverse the props hash a single time after it's merged with shared props
  2. Put the prop filtering logic in a single place

Note: the renderer diff isn't very helpful; better to view the entire file to see how it looks now.

TODO:

  • add some more specs to test the interaction of all these crazy prop filtering types 😅
  • update docs to explain dot notation


drop_partial_except_keys(_props) if rendering_partial_component?
deep_transform_props _props do |prop, path|
next [:dont_keep] unless keep_prop?(prop, path)
Copy link
Collaborator Author

@bknoles bknoles Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why return this array from the block now? Well, the block has two jobs 1) to filter out props we don't need and 2) transform the keepers into their final format. We want track those separately so that the transformation can return things that are nil/false-y. (i think the typical pattern here would be next nil unless some_condition?)

(This block will also be a nice place to rewrite the logic from #154 )


hash.transform_values {|value| deep_transform_values(value, &block)}
end
def deep_transform_props(props, parent_path = '', &block)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar logic as the previous method, but we're now also dealing with the results of the filtering within the hash we're building up and conditionally setting keys.

@@ -138,5 +134,35 @@ def resolve_component(component)

configuration.component_path_resolver(path: controller.controller_path, action: controller.action_name)
end

def keep_prop?(prop, path)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the prop filtering logic now lives in one concise place. i debated pulling this out into a class, but I don't think the cleanliness is worth the extra indirection.

return true if prop.is_a?(AlwaysProp)

if rendering_partial_component?
path_with_prefixes = path_prefixes(path)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth nothing that if we're in a partial reload, we'll end up calculating this array for every single prop key. That should still be way faster than fully evaluating all of those props, though.

Copy link
Contributor

@skryukov skryukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this, looks really clean!

end

def excluded_by_except_partial_keys?(path_with_prefixes)
partial_except_keys.present? && (path_with_prefixes & partial_except_keys.map(&:to_s)).any?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can replace .filter_map(&:to_sym) in the partial_except_keys and then stop calling .map(&:to_s) here

And same for partial_keys

Suggested change
partial_except_keys.present? && (path_with_prefixes & partial_except_keys.map(&:to_s)).any?
partial_except_keys.present? && (path_with_prefixes & partial_except_keys).any?

parts = key.to_s.split('.').map(&:to_sym)
*initial_keys, last_key = parts
current = initial_keys.any? ? hash.dig(*initial_keys) : hash
if prop.is_a?(Hash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that 'what_about_empty_hash' => {}, example will fail, here's a fix:

Suggested change
if prop.is_a?(Hash)
if prop.is_a?(Hash) && prop.any?

end
def deep_transform_props(props, parent_path = '', &block)
props.reduce({}) do |transformed_props, (key, prop)|
current_path = [parent_path, key].reject(&:empty?).join('.')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can simplify this by sticking with arrays:

Suggested change
current_path = [parent_path, key].reject(&:empty?).join('.')
current_path = path + [key]

Copy link

cloudflare-workers-and-pages bot commented Nov 21, 2024

Deploying inertia-rails with  Cloudflare Pages  Cloudflare Pages

Latest commit: c4606ef
Status: ✅  Deploy successful!
Preview URL: https://8d1fa703.inertia-rails.pages.dev
Branch Preview URL: https://dot-notation-for-only-props.inertia-rails.pages.dev

View logs

@bknoles
Copy link
Collaborator Author

bknoles commented Nov 21, 2024

Copy link
Collaborator

@BrandonShar BrandonShar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! only one small comment


drop_partial_except_keys(_props) if rendering_partial_component?
deep_transform_props _props do |prop, path|
next [:dont_keep] unless keep_prop?(prop, path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: could we move these symbol values to constants since they're acting as an enum?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure! as an aside, :dont_keep doesn't do anything at all... it's just there to reveal intention

@bknoles bknoles merged commit e11e8c5 into master Nov 21, 2024
13 checks passed
@bknoles bknoles deleted the dot-notation-for-only-props branch November 21, 2024 15:47
@chasegiunta
Copy link

@bknoles I spent an absurd amount of time troubleshooting why only: ['nested.prop'] was wiping out the entire top-level prop instead of just reloading the nested one, before realizing this isn't a core Inertia feature, so the frontend has no idea the response would need to be merged in.

Is it possible to add this caveat to the rails documentation?

@bknoles
Copy link
Collaborator Author

bknoles commented Jun 27, 2025

Hey @chasegiunta can you describe your situation with some more detail? I need more information to figure out if you've stumbled into a bug or just a misunderstanding we could fix in the docs.

@chasegiunta
Copy link

@bknoles sure. I mean, easiest example would be I have a current_user shared prop. It looks something like this:

current_user: {
 first_name,
 last_name,
 email,
 organizations: { ... },
 preferences: { ... },
 slug
}

Writing a profile form, updating just the user's first & last name (no need to reload all the orgs, preferences, etc again). So we .post() and use only: ['current_user.first_name', 'current_user.last_name']. Doing this will force a reload of the entire current_user object prop and so the only thing returned from Inertia is

current_user: {
  first_name,
  last_name
}

Well, that's not ideal. Now if I have email, orgs, etc displayed elsewhere on the page (not in forms, just displayed in the UI), all that data is lost.

Why it doesn't work makes sense: it's not a Inertia core feature. The Inertia frontend library would have to be aware of which properties being returned in the response are meant to be merged into frontend props. It doesn't currently do that.

@bknoles
Copy link
Collaborator Author

bknoles commented Jun 27, 2025

Oh I see. Yea, partial data reloads overwrite props rather than merging them. The use case for dot notation :only keys in the Laravel adapter's test suite is instructive: the :only data really just serves to filter out a token property that isn't necessary at all on reloads. That's different than not wanting to reload something like organizations while still wanting them around in the UI.

Have you looked at the merging props section of the docs? I think it might be what you're after? Something like:

def create
  render 'Some/Page', props: {
    # :only keys should filter this down to a tiny response; performance benefits depends on how you render the JSON
    current_user: InertiaRails.merge(current_user)
  }
end

(caveat: i haven't built merge props into one of our apps so there may be a nuance my cursory review isn't capturing)

@chasegiunta
Copy link

@bknoles I tried playing with it, yeah, but couldn't quite get it. I think the syntax would actually be current_user: InertiaRails.merge { current_user }, right? (based on looking at the docs, not a ruby dev so forgive me), but we're using a serializer for the current_user anyway. Using only: ['current_user.first_name'] there would just omit current_user entirely in the response

@bknoles
Copy link
Collaborator Author

bknoles commented Jun 27, 2025

Ah yea, merging won't work either now that I think it through. We actually call this out in the docs:

image

The purpose of a partial reload is to prevent unnecessary computations by filtering out props before any callable props are evaluated. InertiaRails.merge() is a callable. From the perspective of the filtering algorithm, there are zero users with a "name" property in this code:

render inertia: 'SomePage', props: {
  users: -> { User.where(some_conditions) }
 }

because the filtering happens before User.where(some_conditions) is evaluated.

Maybe you could use a query parameter to render the user differently within a merge prop (I haven't tested this):

render inertia: 'SomePage', props: {
  current_user: InertiaRails.deep_merge{
    params[:give_me_the_slim_user_please] ? current_user.as_json(only: [:first_name, :last_name]) : current_user
   }
}

then in the JS

router.get(currentPagePath, { give_me_the_slim_user_please: 1 })

Basically, grabbing a subset of an evaluated prop and then merging it back into the Page isn't a workflow that partial reloading is capable of doing right now... so some creativity is required.

I will say that we're talking about this and would like to support complex fetching/merging more cleanly. But nothing ready as of today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for dot notation in :only partial reloads
4 participants